Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[internal] Add experimental_compatible_resolves to python_requirement and use it to generate lockfiles #13978

Merged

Conversation

Eric-Arellano
Copy link
Contributor

Part of the design from #13621. This change has worked well for JVM.

This does have an open question though of how we should determine which interpreter constraints to use for each resolve. python_requirement targets do not have constraints themselves, so we either need to associate each resolve with particular constraints via global config, or we need to look at all consumers of the resolves to calculate it. For now, we just use the global default.

Reminder that these changes are only enabled if you use --python-enable-resolves.

[ci skip-rust]
[ci skip-build-wheels]

…ent` and use it to generate lockfiles

# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
@stuhood
Copy link
Member

stuhood commented Dec 27, 2021

This does have an open question though of how we should determine which interpreter constraints to use for each resolve. python_requirement targets do not have constraints themselves, so we either need to associate each resolve with particular constraints via global config, or we need to look at all consumers of the resolves to calculate it.

Given this, should we instead continue to calculate the resolve based on who is depending on each requirement, and only use the compatible_resolves to guide inference for now? It seems like #13882 will be likely to affect how the interpreter constraints are set, which would avoid adding the field to python_requirement and then immediately changing it.

@Eric-Arellano
Copy link
Contributor Author

I recommend that we either:

  1. lean in this week into figuring out how interpreter constraints + resolve relate, particularly if we want to couple the two into a single "context" vs. keep them both as free variables.
  2. punt on the question and land this PR as is. This feature works well so long as your repo only has a single interpreter constraint. I can continue implementing multiple resolves support as if there is only one set of interpreter constraints, which will make lots of progress.

Imo the status quo is too confusing to try to preserve - don't attempt to handle mixed interpreter constraints with multiple resolves until we've properly designed it.

Given that this is all feature gated and collaboration is mostly async with the holidays, I'd vote for punting, but understand if we want to block. Wdyt?

Copy link
Member

@stuhood stuhood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that this is all feature gated and collaboration is mostly async with the holidays, I'd vote for punting, but understand if we want to block. Wdyt?

Works for me.

@Eric-Arellano Eric-Arellano merged commit 95fc8e1 into pantsbuild:main Dec 27, 2021
@Eric-Arellano Eric-Arellano deleted the python-req-compatible-resolves branch December 27, 2021 22:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants